Skip to content

Comments

doc,test: clarify --eval syntax for leading '-' scripts#61962

Open
jorgitin02 wants to merge 2 commits intonodejs:mainfrom
jorgitin02:fix/43397-cli-eval-unary-negation
Open

doc,test: clarify --eval syntax for leading '-' scripts#61962
jorgitin02 wants to merge 2 commits intonodejs:mainfrom
jorgitin02:fix/43397-cli-eval-unary-negation

Conversation

@jorgitin02
Copy link

@jorgitin02 jorgitin02 commented Feb 24, 2026 •
edited
Loading

Refs: #43397

Summary

  • remove the parser heuristic added in the previous revision for -e -<text>
  • document the supported form for leading-hyphen eval expressions: --eval=<script> (for example --eval=-42)
  • add/adjust tests in test/parallel/test-cli-eval.js to cover --eval=-42, --eval=-0, and keep -e -p as a missing-argument error

Testing

  • python3 tools/test.py test/parallel/test-cli-eval.js
  • python3 tools/test.py --repeat=5 test/parallel/test-cli-eval.js
  • python3 tools/test.py test/parallel/test-cli-bad-options.js
  • make lint-js NODE=/opt/homebrew/bin/node
  • make doc-only NODE=/opt/homebrew/bin/node
  • make lint-md LINT_MD_TARGETS=doc/api/cli.md NODE=/opt/homebrew/bin/node

@jorgitin02
cli: allow unary-negated eval arguments
2086d8c 
Fixes: nodejs#43397
Signed-off-by: jorge guerrero <grrr.jrg@gmail.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. labels Feb 24, 2026
mcollina
mcollina approved these changes Feb 24, 2026
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2026
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/71431/

@codecov
Copy link

codecov bot commented Feb 24, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 31.81818% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (da5efc4) to head (2086d8c).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/node_options-inl.h 31.81% 7 Missing and 8 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #61962      +/-   ##
==========================================
+ Coverage   88.84%   89.73%   +0.89%     
==========================================
  Files         674      672       -2     
  Lines      204957   204545     -412     
  Branches    39309    39276      -33     
==========================================
+ Hits       182087   183557    +1470     
+ Misses      15088    13306    -1782     
+ Partials     7782     7682     -100
Files with missing lines Coverage Δ
src/node_options-inl.h 79.92% <31.81%> (-3.21%) ⬇️

... and 122 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

addaleax
addaleax requested changes Feb 24, 2026
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the ticket:

For users who prefer setting mandatory option arguments rather than positional arguments, node --print --eval=-42 already works. :)

I think that's the right solution here. Adding behavior that depends on the specific text passed to the flag is going to be dangerous; now you cannot know anymore whether node -e -<text> will work or throw, it's going to depend on the specific value of text (i.e. the behavior is less consistent now, not more).

@jorgitin02
doc,test: clarify --eval usage for leading '-' scripts
ad880ae 
Refs: nodejs#43397
Signed-off-by: jorge guerrero <grrr.jrg@gmail.com>
@jorgitin02 jorgitin02 changed the title cli: allow unary-negated eval arguments doc,test: clarify --eval syntax for leading '-' scripts Feb 24, 2026
@jorgitin02
Copy link
Author

jorgitin02 commented Feb 24, 2026

Thanks for the review feedback. I pushed a follow-up that removes the -e -<text> parser heuristic entirely and aligns with the suggested approach from the issue discussion: documenting and testing --eval=<script> for leading-hyphen expressions (for example --eval=-42).

I also kept the safety behavior for -e -p (still a missing-argument error).

Could you please take another look when you have a moment?

@jorgitin02
Copy link
Author

jorgitin02 commented Feb 24, 2026

Heads up: all GitHub Actions workflows for this latest push are currently in action_required state (0s), so no CI jobs have actually started yet.

I don't have permissions to rerun/approve those runs from my fork. Could a maintainer please click Approve and run workflows?

For local verification on my side, I ran:

  • python3 tools/test.py test/parallel/test-cli-eval.js
  • python3 tools/test.py --repeat=5 test/parallel/test-cli-eval.js
  • python3 tools/test.py test/parallel/test-cli-bad-options.js
  • make lint-js NODE=/opt/homebrew/bin/node
  • make doc-only NODE=/opt/homebrew/bin/node
  • make lint-md LINT_MD_TARGETS=doc/api/cli.md NODE=/opt/homebrew/bin/node

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 25, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2026
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 25, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/71448/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@addaleax addaleax addaleax approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jorgitin02 @nodejs-github-bot @mcollina @addaleax